Skip to content

fix: avoid extra compositions due trailing newline - #3084

Merged
wilsonrivera merged 3 commits into
mainfrom
wilson/eng-9866-controlplane-unchanged-subgraph-publish-triggers
Jul 14, 2026
Merged

fix: avoid extra compositions due trailing newline#3084
wilsonrivera merged 3 commits into
mainfrom
wilson/eng-9866-controlplane-unchanged-subgraph-publish-triggers

Conversation

@wilsonrivera

@wilsonrivera wilsonrivera commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes
    • Prevented unnecessary subgraph compositions when schema updates differ only by trailing whitespace/newline formatting.
    • Normalized trailing whitespace when comparing and storing updated schema versions.
  • Tests
    • Added a case ensuring re-publishing with formatting-only schema differences does not trigger extra compositions.
  • Chores
    • Removed an unused newField from the Product test GraphQL schema.

Checklist

Open Source AI Manifesto

This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.

This fixes a regression that caused changes to be detected when only trailing newlines were either added or removed

@wilsonrivera
wilsonrivera requested a review from a team as a code owner July 14, 2026 18:47
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 805086af-bb4c-4862-aced-d31bd0b79914

📥 Commits

Reviewing files that changed from the base of the PR and between dcbd961 and 1b5ec87.

📒 Files selected for processing (1)
  • controlplane/test/test-data/feature-flags/products-standalone-feature.graphql
💤 Files with no reviewable changes (1)
  • controlplane/test/test-data/feature-flags/products-standalone-feature.graphql

Walkthrough

Subgraph schema change detection now trims trailing whitespace before comparison and persistence. Publication tests verify newline-only formatting differences do not trigger additional compositions, and a standalone feature schema removes Product.newField.

Changes

Schema whitespace normalization

Layer / File(s) Summary
Normalize schema changes and validate publication
controlplane/src/core/repositories/SubgraphRepository.ts, controlplane/test/subgraph/publish-subgraph.test.ts
writeSchemaAndCollectAffected compares and stores trimmed SDL, while publication tests cover trailing newline variations and unchanged composition status.
Update feature schema fixture
controlplane/test/test-data/feature-flags/products-standalone-feature.graphql
Removes the newField declaration from the standalone Product schema fixture.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: preventing extra compositions caused by trailing newlines in subgraph schemas.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
controlplane/src/core/repositories/SubgraphRepository.ts (1)

424-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Trailing-whitespace normalization looks correct; consider hoisting the trimmed value.

data.schemaSDL.trimEnd() is computed twice (comparison + storage) and subgraph.schemaSDL.trimEnd() once. Logic is correct and matches the PR's regression fix, but hoisting into a local avoids redundant computation and keeps the comparison/storage value obviously in sync.

♻️ Optional refactor
+    const trimmedSchemaSDL = data.schemaSDL?.trimEnd();
     if (
       data.schemaSDL &&
-      (subgraph.type === 'grpc_plugin' || data.schemaSDL.trimEnd() !== subgraph.schemaSDL.trimEnd())
+      (subgraph.type === 'grpc_plugin' || trimmedSchemaSDL !== subgraph.schemaSDL.trimEnd())
     ) {
       subgraphChanged = true;
       const updatedSubgraph = await subgraphRepo.addSchemaVersion({
         targetId: subgraph.targetId,
-        subgraphSchema: data.schemaSDL.trimEnd(),
+        subgraphSchema: trimmedSchemaSDL!,
         isV2Graph: data.isV2Graph,
         proto: data.proto,
       });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controlplane/src/core/repositories/SubgraphRepository.ts` around lines 424 -
431, In the schema update logic around subgraphRepo.addSchemaVersion, hoist
data.schemaSDL.trimEnd() into a local value and reuse it for both the change
comparison and subgraphSchema assignment, preserving the existing grpc_plugin
condition and normalized comparison behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Around line 424-431: In the schema update logic around
subgraphRepo.addSchemaVersion, hoist data.schemaSDL.trimEnd() into a local value
and reuse it for both the change comparison and subgraphSchema assignment,
preserving the existing grpc_plugin condition and normalized comparison
behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fa7b2a6a-3f11-47f6-ad3d-bb199243616a

📥 Commits

Reviewing files that changed from the base of the PR and between 030b161 and dcbd961.

📒 Files selected for processing (2)
  • controlplane/src/core/repositories/SubgraphRepository.ts
  • controlplane/test/subgraph/publish-subgraph.test.ts

@wilsonrivera
wilsonrivera merged commit 3c15243 into main Jul 14, 2026
11 checks passed
@wilsonrivera
wilsonrivera deleted the wilson/eng-9866-controlplane-unchanged-subgraph-publish-triggers branch July 14, 2026 19:31
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.04%. Comparing base (96663cc) to head (1b5ec87).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3084       +/-   ##
===========================================
+ Coverage   43.72%   71.04%   +27.31%     
===========================================
  Files        1069      335      -734     
  Lines      139516    49334    -90182     
  Branches     7315     6033     -1282     
===========================================
- Hits        61002    35047    -25955     
+ Misses      76661    14261    -62400     
+ Partials     1853       26     -1827     
Files with missing lines Coverage Δ
...lplane/src/core/repositories/SubgraphRepository.ts 91.67% <100.00%> (+0.01%) ⬆️

... and 734 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants